feat(otel): otel sampling support in distributed tracing - #4177
Conversation
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7329a3d7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (decision == DDTRACE_OTEL_SAMPLING_DECISION_PROBABILITY && | ||
| !fields.random_value && !fields.threshold && sample_rate > 0) { | ||
| ddtrace_otel_generate_fields(&fields, generated_random_value, | ||
| generated_threshold, trace_id, | ||
| sampling_priority, sample_rate); |
There was a problem hiding this comment.
Rebuild
ot fields after a local probability decision
When an inbound trace was dropped and a local target_span:any sampling rule makes a new probability decision, any valid inbound rv or th prevents regeneration here. The outbound traceparent can therefore say sampled while the preserved random value remains below the preserved threshold, or a partial member can omit the local threshold entirely, causing downstream OpenTelemetry services to calculate the wrong effective sampling probability. Preserve these fields only for INHERITED; a local probability decision needs fields reconciled with its own rate and final priority.
Useful? React with 👍 / 👎.
| if (members.otel_member) { | ||
| ddtrace_otel_append_member(&limited, members.otel_member, | ||
| members.otel_member_len, &member_count); |
There was a problem hiding this comment.
Reserve space for both owned tracestate members
With a large dd member—possible because propagated Datadog tags default to a 512-byte allowance—the first append can consume nearly the entire tracestate limit and the ot append then returns false. Its failure is ignored, and the later loop deliberately skips that ot member, so the emitted header silently loses the new sampling threshold exactly on traces carrying substantial propagated tags. The limiting pass should reserve capacity for both owned members or deliberately trim/evict lower-priority data.
Useful? React with 👍 / 👎.
| // W3C Trace Context permits at most 32 list-members. Owned members | ||
| // are removed or normalized after this pass, then protected when | ||
| // the outbound header is rebuilt. | ||
| if (last_comma && ++commas == 32) { |
There was a problem hiding this comment.
Reapply tracestate limits when constructing span links
Allowing all 32 extracted members is safe only on the outbound-header path, which runs the limiter again after rebuilding the dd member. DDTrace\SpanLink::fromHeaders() and the restart behavior instead pass this result to ddtrace_build_span_link_from_result(), where ddtrace_format_tracestate() can prepend a dd member for an extracted Datadog origin or tags without another limiting pass. An otherwise valid 32-member or 512-byte tracecontext input can therefore produce an over-limit traceState on the span link; retain the previous reservation or normalize the rebuilt link tracestate too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This doesn't really need to be increased to 32, we just should handle "ot" as well and not increase commas for the "ot" case.
There was a problem hiding this comment.
Fixed in 53e88b1; dd and ot are excluded from the 30 vendor-member count so both owned slots remain available.
| span->otel_sampling_rate = sample_rate; | ||
| span->otel_sampling_decision = mechanism == DD_MECHANISM_MANUAL || mechanism == DD_MECHANISM_ASM || limited || sample_rate <= 0 | ||
| ? DDTRACE_OTEL_SAMPLING_DECISION_NON_PROBABILITY | ||
| : DDTRACE_OTEL_SAMPLING_DECISION_PROBABILITY; |
There was a problem hiding this comment.
Treat limiter-approved traces as non-probability decisions
When DD_TRACE_SAMPLE_RATE enables the trace rate limiter and the limiter approves the current candidate, limited is false, so this records a pure probability decision and emits a threshold based only on sample_rate. At throughput above DD_TRACE_RATE_LIMIT, however, admission also depends on the token-bucket limiter: for example, approved traces at sample rate 1 emit th:0 even though many equivalent traces are rejected by the limiter. Downstream OpenTelemetry metrics then treat the surviving spans as unsampled and undercount traffic; any decision for which the active limiter was consulted must omit the probability threshold (or encode the combined effective probability), not only decisions the limiter rejected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The RFC says the limiter changes attribution only when it converts a keep into a drop.
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. If you need to update snapshots, please refer to CONTRIBUTING.md 2 occurrences of : - "_dd.span_links": "[{\"trace_id\":\"0000000000000000c151df7d6ee5e2d6\",\"span_id\":\"c151df7d6ee5e2d6\",\"trace_state\":\"dd=t.dm:-0\",\"attributes\":{\"_dd.p.dm\":\"-0\"}}]"
+ "_dd.span_links": "[{\"trace_id\":\"0000000000000000c151df7d6ee5e2d6\",\"span_id\":\"c151df7d6ee5e2d6\",\"trace_state\":\"dd=t.dm:-0,ot=rv:ea5f0f7db45435;th:0\",\"attributes\":{\"_dd.p.dm\":\"-0\"}}]"
|
| zend_string *normalized_tracestate = ddtrace_otel_sampling_update_tracestate( | ||
| result.tracestate, trace_id.low, result.priority_sampling, DDTRACE_OTEL_SAMPLING_DECISION_INHERITED, 0); |
There was a problem hiding this comment.
It's odd to me to do a two-step normalization here. There's no reason why the otel parsing cannot be inlined in the loop above, producing a single normalization pass.
There was a problem hiding this comment.
result.tracestate is supposed to contain the values untouched by us - i.e. other vendors, which get appended (as it should - new values should come first), ddtrace_otel_sampling_update_tracestate however manipulates the string in place every time it touches it.
Let's please not store individual data (like the otel rv and th) inside the string after parsing. Store it here on result, make it properly available on the span (for inspection / manual changing / interaction with otel sdk), and rebuild it alongside the dd= tags in ddtrace_format_tracestate.
There was a problem hiding this comment.
It should also help keeping the implementation tidier, keeping scanning separate from formatting.
There was a problem hiding this comment.
Also, in fact, you're storing otel_sampling_rate on the span, but never storing the parsed values there, which strikes me as odd.
Because, in fact, the otel_sampling_rate seems to always be equal to the datadog sampling rate (making it redundant as is??)
There was a problem hiding this comment.
Fixed in 89ab3d4:
- it now parses and removes
otduring the existing normalization pass. - parsed OTel fields are stored separately on the trace/span and rebuilt during formatting.
- extraction now parses state, while injection handles formatting.
- parsed values are stored on the span and the redundant otel_sampling_rate was removed.
| if (sample_rate >= 1) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Redundant condition with the if just below?
There was a problem hiding this comment.
Yes, it was redundant and has been removed in 189d449
| ddtrace_otel_encode_56_bit_hex(random_value_int, random_value); | ||
| ddtrace_otel_encode_56_bit_hex(threshold_value, threshold); |
There was a problem hiding this comment.
If you'd print it directly to the smart_str, you could use smart_str_append_printf(&result, "rv:%014x", random_value) instead of reimplementing hex-printing. (and then remove trailing zeroes on the threshold)
There was a problem hiding this comment.
Done in af34e57; sampling fields are now formatted directly with smart_str_append_printf
35dfbcb to
3f39364
Compare
Description
Add support for OTel sampling in distributed tracing, by reading and emitting ot.th and ot.rv tags. These tags will be forwarded to services using OTel SDKs, that should forward them to the backend.
Distributed tracing will work between DD-instrumented services and OTel-instrumented services, downstream OTel services will be able to forward the th field it to the collector and backend, which will calculate metrics correctly
Locally passes DataDog/system-tests#7518
Reviewer checklist